Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUILD] error: read-only reference ‘value’ used as ‘asm’ output #2354

Merged

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Oct 9, 2023

Fixes #2343

Changes

The benchmark code fails to build with GCC, due to an upstream bug.

As a work around, comment the two failing benchmarks.

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@marcalff marcalff requested a review from a team October 9, 2023 14:08
@marcalff marcalff changed the title [Build] error: read-only reference ‘value’ used as ‘asm’ output [BUILD] error: read-only reference ‘value’ used as ‘asm’ output Oct 9, 2023
@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #2354 (ed66352) into main (0eaa794) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2354   +/-   ##
=======================================
  Coverage   87.41%   87.41%           
=======================================
  Files         199      199           
  Lines        6018     6018           
=======================================
  Hits         5260     5260           
  Misses        758      758           

@lalitb
Copy link
Member

lalitb commented Oct 9, 2023

I thought commenting out the failing test for time being was the better approach :) Not super sure if we should ditch the const on those member variables because of a bug somewhere else This would in general prevent any optimizations from compiler, and also type safety.

@marcalff
Copy link
Member Author

marcalff commented Oct 9, 2023

I thought commenting out the failing test for time being was the better approach :) Not super sure if we should ditch the const on those member variables because of a bug somewhere else This would in general prevent any optimizations from compiler, and also type safety.

The const itself is highly suspicious.

Members like:

  • const char *ptr;
  • const & int;

make sense, but I have never seen a member using:

  • const std::string str;

If the member is really const, how is it properly destroyed by the destructor for example ?

I think there is confusion in this code, if an object is const, the entire instance should be const (as in const ParentBasedSampler *sampler), using a non const ParentBasedSampler object which has a const member makes no sense (to me).

@marcalff
Copy link
Member Author

marcalff commented Oct 9, 2023

Taking this for example:

const std::string description_;

The method GetDescription() is const, so it does not change the object, this part is expected.

Making the member const is rather strange.

@marcalff
Copy link
Member Author

marcalff commented Oct 9, 2023

Reverted to first solution, comment out code that fails to build.

@marcalff marcalff mentioned this pull request Oct 9, 2023
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for troubleshooting, and fixing it.

@marcalff marcalff merged commit 05b26ca into open-telemetry:main Oct 9, 2023
45 checks passed
@marcalff marcalff deleted the fix_benchmark_build_break_2343 branch October 27, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build error: read-only reference ‘value’ used as ‘asm’ output
2 participants